add ArrayField #197
add ArrayField #197
Conversation
timgraham
force-pushed
the
arrayfield
branch
2 times, most recently
from
e7088d1 to
6338305
Compare
timgraham
force-pushed
the
arrayfield
branch
4 times, most recently
from
6a97c8e to
8a7d407
Compare
| Specifies the underlying data type and behavior for the array. It | ||
| should be an instance of a subclass of |
There was a problem hiding this comment.
I'm assuming we cannot confirm embedded models until after the embedded model PR is merged.
There was a problem hiding this comment.
I plan to merge this first, then return to EMF and decide what to do.
django_mongodb/fields/array.py
Outdated
| if isinstance(value, list | tuple): | ||
| # Workaround for https://code.djangoproject.com/ticket/35982 | ||
| if isinstance(self.base_field, DecimalField): | ||
| return [self.base_field.get_db_prep_save(i, connection) for i in value] | ||
| return [self.base_field.get_db_prep_value(i, connection, prepared=False) for i in value] | ||
| return value |
There was a problem hiding this comment.
I see the ticket is now marked as resolved and the change will now work with get_db_prep_value. Is this fix now a part of 5.1 or we still need to hold onto this workaround?
There was a problem hiding this comment.
It's fixed in Django 5.2. (This PR still targets Django 5.0.)
django_mongodb/fields/array.py
Outdated
| "$eq": [ | ||
| { | ||
| "$cond": { | ||
| "if": {"$eq": [lhs_mql, None]}, | ||
| "then": False, | ||
| "else": {"$setIsSubset": [value, lhs_mql]}, | ||
| } | ||
| }, | ||
| True, | ||
| ] |
There was a problem hiding this comment.
could we not use $all here?
Here's the equivalent:
{ "$all": value }There was a problem hiding this comment.
I tried it but where does lhs_mql go? return {lhs_mql: {"$all": value}} gives, for example, {'$match': {'$expr': {'$field': {'$all': [{'$literal': 2}]}}}} but MongoDB reports "Unrecognized expression '$field'". I think $all can only be used for find, not in aggregate.
django_mongodb/forms/array.py
Outdated
| super().__init__(**kwargs) | ||
| if min_length is not None: | ||
| self.min_length = min_length | ||
| self.validators.append(ArrayMinLengthValidator(int(min_length))) |
There was a problem hiding this comment.
this can come as anything other than int | None I or is this just an easy way to raise a type validation with int casting?
There was a problem hiding this comment.
I guess so. This doesn't look like typical Django code. As I recall, this code was merged a bit hastily as part of a massive PR during a sprint in 2014 without my careful review. I haven't reviewed the forms side of this PR carefully yet, and while doing so, I'll remove these casts (and probably propose they be removed upstream as well, considering there are no test failures without them).
edit: upon further examination, it was copied from CharField, and yes, it's for type validation: https://code.djangoproject.com/ticket/20440
django_mongodb/forms/array.py
Outdated
| def validate(self, value): | ||
| super().validate(value) | ||
| errors = [] | ||
| for index, item in enumerate(value): | ||
| try: | ||
| self.base_field.validate(item) | ||
| except ValidationError as error: | ||
| errors.append( | ||
| prefix_validation_error( | ||
| error, | ||
| prefix=self.error_messages["item_invalid"], | ||
| code="item_invalid", | ||
| params={"nth": index + 1}, | ||
| ) | ||
| ) | ||
| if errors: | ||
| raise ValidationError(errors) |
There was a problem hiding this comment.
Looks like the validate functions here share a lot with the field class version. Any chance we could make a low-pri task for a mixin class?
There was a problem hiding this comment.
There are subtle differences. For example, the signature of validate() is different between form fields and model fields.
I wouldn't advise using mixins to share functionality between model fields and form fields because django.forms and django.db.models (and this project's corresponding modules) are loosely coupled.
timgraham
force-pushed
the
arrayfield
branch
2 times, most recently
from
6cf9766 to
08ae4b4
Compare
| def as_mql(self, compiler, connection): | ||
| lhs_mql = process_lhs(self, compiler, connection) | ||
| value = process_rhs(self, compiler, connection) | ||
| return {"$and": [{"$ne": [lhs_mql, None]}, {"$setIsSubset": [lhs_mql, value]}]} |
There was a problem hiding this comment.
Debugin the test model_fields_.test_arrayfield.QueryingTests.test_contains_subquery I find that the value must be checked for nullability also, sometimes subqueries will pass None as a value.
return {"$and": [{"$ne": [lhs_mql, None]}, {"$ne": [value, None]}, {"$setIsSubset": [value, lhs_mql]}]}
There was a problem hiding this comment.
Btw, this PR could be merge as it is. that bug could be fixed latter. Just decide to fix this future issue now, or with the corresponding issue
There was a problem hiding this comment.
Thanks. I figured contained_by had the same issue but the fix didn't work. Please take a look if you like.
There was a problem hiding this comment.
🤔 I believe we found a MongoDB bug. 😬 the changes that you made should work, the other thing that I tried is to use the operator $isArray instead of checking for null, don't know if you want an exception if the one of the side isn't a array or a silent empty result.
There was a problem hiding this comment.
Attached Issue: https://jira.mongodb.org/browse/SERVER-99186
Based on
django.contrib.postgres.fields.ArrayField.